fix(cli): validate captured image and font downloads - #3809

The byte-derived extensions, Win32-safe font naming, stream cancellation, and parser-based SVG policy are the right boundaries. I verified the three focused validator suites locally (23/23), including the exact bytes-at-limit and CSS escape cases.

Blocker — embedded raster data bypasses the image validator. packages/cli/src/capture/captureImageValidation.ts:11-15 accepts any base64-shaped data:image/png|jpeg|gif|webp string without decoding or checking its claimed type/pixel bounds, and the SVG arm returns at :132 after that lexical check. Concrete probe: <image href="data:image/png;base64,QUFBQUFBQUFB"/> is accepted as .svg even though those bytes are not a PNG. More importantly, a small compressed raster with huge decoded dimensions bypasses the 40M-pixel ceiling and is then interpreted automatically by the unchanged downstream SVG renderers at packages/cli/src/capture/contactSheet.ts:321 and packages/cli/src/capture/contentExtractor.ts:572. Validate every embedded raster against its declared MIME with a bounded decoded-size/pixel check (and cap total embedded bytes), or reject data-image embeds; add wrong-magic and decompression-bomb controls.

Important — the 75 MiB limit is per response, not per capture. packages/cli/src/capture/assetDownloader.ts:707 applies the video-sized ceiling to every icon/image/font. Five image fetches run concurrently at :398-430, so the chunk arrays plus Buffer.concat can transiently approach 750 MiB for one batch; the 30-font loop at :536-591 can persist 2.25 GiB during the 120s capture budget. That is finite but still a practical remote memory/disk exhaustion path. Before using boundedness as evidence for #886/#887/#877/#878, use type-appropriate limits and a shared aggregate byte budget (exact thresholds can come from real capture fixtures).

Important compatibility edge — canonicalization can collapse two fonts. captureFontValidation.ts:49-57 returns only stem + canonicalExtension and the caller at assetDownloader.ts:588-592 has no used-name set. site.ttf served as WOFF2 and site.woff2 both become site.woff2; likewise two directories/query variants with the same basename. The later write replaces the earlier bytes and both CSS URLs are rewritten to the same file. Preserve the ordinary name when unique, but add deterministic collision handling and a two-distinct-font regression.

Fallow: nonblocking. The only complexity finding is inherited deriveAssetName (blame predates this PR). Both clone groups are test-only fixture/setup repetition; neither duplicates a production decision, so I would not expand scope for them.

I am not approving dispositions yet: #886/#887/#877/#878 still represent intentional persistence, but the SVG nested-payload and aggregate-resource controls have to close before “validated content” is sufficient evidence for scoped false-positive/accepted-capability classifications. CI and the JavaScript CodeQL scan were still running at review time.

— Magi

Verdict: REQUEST CHANGES
Reasoning: The top-level type/path validation is strong, but embedded raster bytes bypass it and the shared response ceiling is still multiplicative enough to exhaust capture resources; font canonicalization also introduces a deterministic overwrite case.

The code blockers from review 5153057215 are resolved at 889c2b6d31b1.

• captureImageValidation.ts:137-185 now collects every embedded raster reference, validates decoded bytes against the declared MIME, counts animated pages into shared 10 MiB/40M-pixel budgets, and decodes accepted content. I re-ran the former wrong-magic probe (now rejected) and an 84-page 1280×720 WebP embed (aggregate 77.4M pixels, now rejected).
• readBoundedResponse.ts:1-42 owns the 100 MiB capture budget and debits actual chunks synchronously across concurrent readers; assetDownloader.ts:202,420-425,490-495,605-610 applies the 2/20/10 MiB family ceilings. The same budget instance is created once at capture/index.ts:612 and passed through both font and image passes.
• captureFontValidation.ts:61-71 reserves names case-insensitively and deterministically suffixes collisions; the end-to-end two-real-font test proves both files and both CSS rewrites survive.

Local exact-head verification: four focused files, 65/65 tests pass. The six reviewer regressions genuinely fail the prior head by construction: wrong/mismatched embedded MIME, aggregate embedded pixels, concurrent/shared byte budgets, and unit/end-to-end font collisions. Fallow remains nonblocking: inherited deriveAssetName complexity; no production clone group. The PR body now matches the 2/20/10/100 MiB limits and 268-test result.

CodeQL rule js/http-to-file-access asks for an intent audit rather than a prescribed sanitizer. The exact PR merge-ref JavaScript analysis 1747215971 reports 0 results, and the PR-ref open-alert set is empty. Per original IDs, no manual dismissal is warranted:

• #886: fixed — 2 MiB icon, decoded/canonical type, passive SVG boundary, safe generated stem.
• #887: fixed/dependent — copies only #886-validated bytes to the canonical fixed favicon name.
• #877: fixed — 20 MiB + aggregate budget, decoded image/passive SVG, fixed stem + canonical extension.
• #878: fixed — 10 MiB + aggregate budget, container/fontkit/expanded-size validation, Windows-safe collision-free name.

Those should close automatically as fixed after merge; do not dismiss them pre-merge. If any remains after the main scan, the same evidence supports a scoped false-positive disposition, but only after confirming its source/sink is unchanged.

I am not lifting the old CHANGES_REQUESTED yet because the required Tests on windows-latest aggregate is red: its first exact-head run was externally canceled while render was at frame 351/360 and the Studio lanes were still progressing, so this is not a code failure, but the replacement run 34342425236 is still in progress. The main required Test aggregator is also pending. Re-request once those native gates settle; source is ready for approval.

— Magi

Verdict: COMMENT
Reasoning: All substantive review findings are fixed and the exact-head security evidence is clean, but required Windows/aggregate checks have not yet passed, so approval must wait for the native gates.

All substantive findings from review 5153057215 remain resolved at unchanged head 889c2b6d31b1, as documented with exact source and local evidence in review 5153323778.

The external report now matches GitHub: replacement Windows run 34342425236 is fully successful, CI 34342425320 is successful including the required Test aggregator, all required checks pass, the head has no failing check runs, and CodeQL 34341826280 / JS analysis 1747215971 is successful with zero PR-ref results and no open PR alerts. #886/#887/#877/#878 should close automatically as fixed on the main scan; no dismissal is warranted before merge.

Fallow assessment is unchanged and nonblocking: inherited deriveAssetName complexity and test-only repetition, with no duplicated production decision.

— Magi

Verdict: APPROVE
Reasoning: The security, resource-bound, and font-fidelity blockers are fixed at this exact head, and every native required gate plus CodeQL is now green.